Enable Branch Indexing#151
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the HoloViz MCP servers and documentation indexer to improve tool registration consistency and to better respect configured git refs (branch/tag) when cloning/updating documentation repositories.
Changes:
- Add unit tests covering git clone/update behavior when
branch/tagare configured. - Update MCP tool decorators across servers from
@mcp.toolto@mcp.tool(). - Adjust
DocumentationIndexer._clone_or_update_repo_syncto detect stale checkouts and re-clone when the configured ref changes.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/docs_mcp/test_data.py |
Adds tests for branch/tag clone/update behavior. |
src/holoviz_mcp/holoviz_mcp/data.py |
Implements ref-aware clone/update logic and minor log formatting tweaks. |
src/holoviz_mcp/holoviz_mcp/server.py |
Switches to @mcp.tool() and comments out the update_index tool decorator. |
src/holoviz_mcp/panel_mcp/server.py |
Switches tools to @mcp.tool(). |
src/holoviz_mcp/hvplot_mcp/server.py |
Switches tools to @mcp.tool(). |
src/holoviz_mcp/holoviews_mcp/server.py |
Switches tools to @mcp.tool(). |
Comments suppressed due to low confidence (1)
src/holoviz_mcp/holoviz_mcp/server.py:356
update_index’s docstring and examples describe it as an MCP tool, but the@mcp.tool(enabled=False)decorator has been commented out, so it’s no longer registered at all. Either restore the decorator (if you want it present-but-disabled) or adjust/remove the docstring/examples to avoid advertising a tool that can’t be called.
# @mcp.tool(enabled=False)
async def update_index(ctx: Context) -> str:
"""Update the documentation index by re-cloning repositories and re-indexing content.
DO use this tool periodically (weekly) to ensure the documentation index is up-to-date
| expected_ref = repo_config.branch or repo_config.tag | ||
|
|
||
| if expected_ref: | ||
| try: | ||
| current_ref = repo.active_branch.name | ||
| except TypeError: | ||
| current_ref = None # Detached HEAD (e.g. tag/commit checkout) | ||
|
|
||
| if current_ref != expected_ref: | ||
| logger.info(f"Stale checkout for {repo_name}: on '{current_ref}', expected '{expected_ref}'. " "Deleting and re-cloning...") | ||
| shutil.rmtree(repo_path) | ||
| # Fall through to clone below | ||
| else: | ||
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | ||
| repo.remotes.origin.pull() | ||
| return repo_path | ||
| else: | ||
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | ||
| repo.remotes.origin.pull() | ||
| return repo_path |
There was a problem hiding this comment.
Using repo.active_branch to determine the current ref breaks for tag-based clones: cloning a tag via --branch <tag> typically leaves the repo in a detached HEAD where active_branch raises and current_ref becomes None, causing unnecessary re-clones (or always re-cloning) even when the correct tag is already checked out. Consider handling repo_config.tag separately by comparing the checked-out commit to the tag’s commit (or using an exact-match tag check), and avoid relying on active_branch for tags.
| expected_ref = repo_config.branch or repo_config.tag | |
| if expected_ref: | |
| try: | |
| current_ref = repo.active_branch.name | |
| except TypeError: | |
| current_ref = None # Detached HEAD (e.g. tag/commit checkout) | |
| if current_ref != expected_ref: | |
| logger.info(f"Stale checkout for {repo_name}: on '{current_ref}', expected '{expected_ref}'. " "Deleting and re-cloning...") | |
| shutil.rmtree(repo_path) | |
| # Fall through to clone below | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path | |
| # If a tag is specified, avoid relying on active_branch (HEAD will be detached). | |
| if repo_config.tag: | |
| tag_name = repo_config.tag | |
| tag_ref = next((t for t in repo.tags if t.name == tag_name), None) | |
| if tag_ref is not None and repo.head.commit == tag_ref.commit: | |
| # Already at the expected tag commit; nothing to update for immutable tags. | |
| logger.info( | |
| f"Repository {repo_name} at {repo_path} already checked out at tag '{tag_name}'." | |
| ) | |
| return repo_path | |
| logger.info( | |
| f"Stale or mismatched tag checkout for {repo_name}: expected tag '{tag_name}'. " | |
| "Deleting and re-cloning..." | |
| ) | |
| shutil.rmtree(repo_path) | |
| # Fall through to clone below | |
| else: | |
| # Branch-based (or default) checkout: use active_branch when available. | |
| expected_ref = repo_config.branch | |
| if expected_ref: | |
| try: | |
| current_ref = repo.active_branch.name | |
| except Exception: | |
| # Detached HEAD or other issue; treat as unknown branch. | |
| current_ref = None | |
| if current_ref != expected_ref: | |
| logger.info( | |
| f"Stale checkout for {repo_name}: on '{current_ref}', expected '{expected_ref}'. " | |
| "Deleting and re-cloning..." | |
| ) | |
| shutil.rmtree(repo_path) | |
| # Fall through to clone below | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path |
| expected_ref = repo_config.branch or repo_config.tag | ||
|
|
||
| if expected_ref: | ||
| try: | ||
| current_ref = repo.active_branch.name | ||
| except TypeError: | ||
| current_ref = None # Detached HEAD (e.g. tag/commit checkout) | ||
|
|
||
| if current_ref != expected_ref: | ||
| logger.info(f"Stale checkout for {repo_name}: on '{current_ref}', expected '{expected_ref}'. " "Deleting and re-cloning...") | ||
| shutil.rmtree(repo_path) | ||
| # Fall through to clone below | ||
| else: | ||
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | ||
| repo.remotes.origin.pull() | ||
| return repo_path | ||
| else: | ||
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | ||
| repo.remotes.origin.pull() | ||
| return repo_path |
There was a problem hiding this comment.
expected_ref only considers branch/tag, so a repo configured with a specific commit will be treated as having no expected ref when it already exists and will just pull(), potentially moving HEAD away from the configured commit. If repo_config.commit is meant to pin the checkout, add a commit check for existing repos (e.g., compare repo.head.commit.hexsha and reset/re-clone when it differs).
| expected_ref = repo_config.branch or repo_config.tag | |
| if expected_ref: | |
| try: | |
| current_ref = repo.active_branch.name | |
| except TypeError: | |
| current_ref = None # Detached HEAD (e.g. tag/commit checkout) | |
| if current_ref != expected_ref: | |
| logger.info(f"Stale checkout for {repo_name}: on '{current_ref}', expected '{expected_ref}'. " "Deleting and re-cloning...") | |
| shutil.rmtree(repo_path) | |
| # Fall through to clone below | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path | |
| # If a specific commit is configured, treat it as a pinned checkout. | |
| if repo_config.commit: | |
| current_commit = repo.head.commit.hexsha | |
| expected_commit = repo_config.commit | |
| # Allow expected_commit to be a short SHA prefix. | |
| if not current_commit.startswith(expected_commit): | |
| logger.info( | |
| f"Stale checkout for {repo_name}: on commit '{current_commit}', " | |
| f"expected '{expected_commit}'. Deleting and re-cloning..." | |
| ) | |
| shutil.rmtree(repo_path) | |
| # Fall through to clone below | |
| else: | |
| logger.info( | |
| f"Repository {repo_name} already at desired commit '{expected_commit}'." | |
| ) | |
| return repo_path | |
| else: | |
| expected_ref = repo_config.branch or repo_config.tag | |
| if expected_ref: | |
| try: | |
| current_ref = repo.active_branch.name | |
| except TypeError: | |
| current_ref = None # Detached HEAD (e.g. tag/commit checkout) | |
| if current_ref != expected_ref: | |
| logger.info( | |
| f"Stale checkout for {repo_name}: on '{current_ref}', expected '{expected_ref}'. " | |
| "Deleting and re-cloning..." | |
| ) | |
| shutil.rmtree(repo_path) | |
| # Fall through to clone below | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path | |
| else: | |
| logger.info(f"Updating {repo_name} repository at {repo_path}...") | |
| repo.remotes.origin.pull() | |
| return repo_path |
|
|
||
| mock_repo = MagicMock() | ||
| mock_repo.active_branch.name = "v0.9.0" | ||
|
|
There was a problem hiding this comment.
These tag tests don’t reflect GitPython behavior for tag checkouts: in a tag-based clone the repo is usually in detached HEAD and accessing active_branch raises TypeError. As written (mock_repo.active_branch.name = ...), the test can’t catch the detached-HEAD path and will miss the real bug. Update the mock to raise on active_branch and assert the intended tag detection behavior (including a case where the correct tag is already checked out and does not trigger an rmtree/re-clone).
Before you could not index branches